API review

Proposer: Blaise Gassend

Present at review:

• List reviewers
  • Ken

What to review

dynparam

Dynparam has been designed to be as similar as possible to rosparam. Try it out by running testserver in dynamic_reconfigure and then issuing commands:

rosrun dynamic_reconfigure dynparam ...

Python client API

Have a look at:

• 'DynamicReconfigureClient' in 'dynamic_reconfigure/src/py_dynamic_reconfigure.py'

• 'test/testclient.py'

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

kwc:

• python API
  • dynamic_reconfigure vs. py_dynamic_reconfigure is completely confusing. Why can't it just be dynamic_reconfigure?

  • The stable API itself seems easy enough. "DynamicReconfigureClient" is a bit of a mouthful to type. You might just be able to call it "Client".

  • py_dynamic_reconfigure shouldn't be raising generic Exceptions. It should raise its own exception class so that it can be caught separately. It also needs to document that it raises Exceptions.
  • This level of API review is as much of a review of the presence of API documentation, so:
    • Wiki page at the very least needs a basic overview of what are the accessible APIs (i.e. "dynamic_reconfigure has a Python client API...")
    • I added a rosdoc.yaml so that epydoc would generate, and I also tweaked the epydoc.config
    • I deleted the placeholder mainpage.dox

    • ConfigType.py should only use single # comments. Double-pound comments show up in Doxygen

    • I made some tweaks to the epydoc comments.
    • The API isn't abundantly clear about what I should and shouldn't use. The "Please do not use..." at the end of the comment doesn't scan immediately, and quite a lot of the API is undocumented with no examples (I assume that these are do not use as well). Perhaps just putting "UNSTABLE" at the start of the comment would be easier.
• dynparam:
  • why is there both "set" and "paramset"? I don't understand the difference given the current documentation
  • paramset usage is wrong (it doesn't mention parameter as input, nor describe the explain that you use the _private_param assign syntax)

  • BUG: dynparam does not appear to catch Exceptions e.g.

    kwc@ann:/home/kwc/ros-pkg/ros-pkg-trunk/stacks/driver_common/dynamic_reconfigure/scripts$ ./dynparam set dynamic_reconfigure_test_server foo 1
    Traceback (most recent call last):
      File "./dynparam", line 186, in <module>
        elif cmd == "set":      do_set()
      File "./dynparam", line 80, in do_set
        set_params(node, { parameter : value }, timeout=options.timeout)
      File "./dynparam", line 155, in set_params
        config = client.update_configuration(params)
      File "/home/kwc/ros-pkg/ros-pkg-trunk/stacks/driver_common/dynamic_reconfigure/src/py_dynamic_reconfigure.py", line 203, in update_configuration
        raise Exception('don\'t know type for parameter: %s' % name)
    Exception: don't know type for parameter: foo

  • dynparam set/dump/etc... on an invalid node name hangs forever. I would expect it to fail or at least tell me that it's waiting forever (e.g. "WARNING: node 'foo' is not online yet, waiting for it to come online"). I know that there is a separate timeout option, but this is still confusing. I'd rather the default timeout be zero and have to explicitly enable the waiting.
  • setting -t 0 should return immediately, but instead blocks
  • why does "dump" format not correspond to get format? i.e. it also includes node name.

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

Py API

• Change py_dynamic_reconfigure to dynamic_reconfigure.
  • Done. Only external import was from pr2_camera_synchronizer/synchronizer_classes.py.
• Rename DynamicReconfigureClient/Server to Client/Server.
  • Done.
• Change to non-generic exceptions.

  • Done. Added DynamicReconfigureException classes.

• Mark unstable stuff as plain UNSTABLE.
  • Done. Prefixed method docs with "UNSTABLE".

dynparam

• Document paramset better. Change "paramset" to ???

  • Done. paramset -> set_from_parameters

• Add warning if node is not up, send to stderr.
  • Done.
• Fix -t 0 behavior.
  • Done. -t 0 is now non-blocking.
• Get get and set to match again.
  • Done. set now also takes yaml input for setting multiple parameters at once.

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved